Skip to content

OCPSTRAT-1677: feat: allow for aws spot market options on node pools - #6707

Closed
gdbranco wants to merge 1 commit into
openshift:mainfrom
gdbranco:feat/ocpstrat-1677
Closed

OCPSTRAT-1677: feat: allow for aws spot market options on node pools#6707
gdbranco wants to merge 1 commit into
openshift:mainfrom
gdbranco:feat/ocpstrat-1677

Conversation

@gdbranco

@gdbranco gdbranco commented Aug 27, 2025

Copy link
Copy Markdown

feat(aws): add AWS Spot instance support to NodePools

This commit introduces comprehensive support for AWS Spot instances in HyperShift NodePools:

- Add AWSSpotMarketOptions type with maxPrice configuration
- Implement validation rules to prevent incompatible placement configurations
- Update generated client code and API documentation
- Add comprehensive end-to-end tests for validation scenarios
- Include detailed documentation for Spot instance usage

Key features:
- Optional maxPrice field to control maximum Spot instance pricing
- Automatic compatibility validation (no dedicated tenancy or capacity reservations)
- Integration with existing NodePool auto-repair functionality for instance replacement
- Support for cost-effective node provisioning with proper interruption handling

Fixes #OCPSTRAT-1677

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

Summary by CodeRabbit

  • New Features

    • Add AWS Spot support for NodePools via spotMarketOptions with optional maxPrice (validated numeric string); NodePools launched as Spot will be recognized and treated accordingly.
  • Validation

    • Enforce that spotMarketOptions is incompatible with Capacity Reservations and requires tenancy to be unset or "default"; explicit validation messages guide users.
  • Documentation

    • New how-to guide for running NodePools on AWS Spot, examples, best practices, and verification steps.
  • Tests

    • Expanded unit, integration, and e2e tests covering Spot behavior, tenancy rules, and incompatibility cases.

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 27, 2025
@openshift-ci

openshift-ci Bot commented Aug 27, 2025

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai

coderabbitai Bot commented Aug 27, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Adds AWSSpotMarketOptions with MaxPrice to the AWS placement API and CRDs, extends PlacementOptions with spotMarketOptions and CEL cross-field validations, updates controller reconciliation/validation to emit SpotMarketOptions, and adds apply-config, deepcopy, unit/e2e tests, and documentation.

Changes

Cohort / File(s) Summary
API: Type and validation additions
api/hypershift/v1beta1/aws.go, vendor/.../api/hypershift/v1beta1/aws.go
Add public type AWSSpotMarketOptions{MaxPrice *string} and PlacementOptions.SpotMarketOptions *AWSSpotMarketOptions; add CEL x-kubernetes-validations enforcing mutual exclusivity with capacityReservation and tenancy requirements.
Generated deepcopy
api/hypershift/v1beta1/zz_generated.deepcopy.go, vendor/.../zz_generated.deepcopy.go
Add DeepCopyInto/DeepCopy for AWSSpotMarketOptions; update PlacementOptions deepcopy to handle SpotMarketOptions.
Client apply-configuration
client/applyconfiguration/hypershift/v1beta1/awsspotmarketoptions.go, client/applyconfiguration/hypershift/v1beta1/placementoptions.go, client/applyconfiguration/utils.go
Add AWSSpotMarketOptionsApplyConfiguration with WithMaxPrice; add PlacementOptionsApplyConfiguration.SpotMarketOptions and setter; extend ForKind switch.
CRDs (generated manifests)
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/*.crd.yaml, api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/*
Add spec.platform.aws.placement.spotMarketOptions with maxPrice pattern/maxLength and x-kubernetes-validations enforcing incompatibility with capacityReservation and tenancy constraints; retain tenancy enum and host-capacityReservation rule.
Controller logic
hypershift-operator/controllers/nodepool/aws.go
Reconcile sets AWSMachineTemplate.Spec.SpotMarketOptions (MarketType=Spot; set MaxPrice if present); add validation to reject spotMarketOptions with non-default/host tenancy or with capacityReservation; add nil AWS platform guard.
Unit tests
hypershift-operator/controllers/nodepool/aws_test.go
Add tests for propagation of SpotMarketOptions and MarketType=Spot; expand validation tests for tenancy and capacityReservation interactions; refactor test harness.
E2E tests
test/e2e/nodepool_spotmarketoptions_test.go, test/e2e/nodepool_test.go, test/e2e/create_cluster_test.go
Add end-to-end success test for Spot config; insert test into suite; add API validation test cases asserting CEL validation messages for spot vs tenancy/capacityReservation.
Docs
docs/content/how-to/automated-machine-management/aws-spot-instances.md, docs/content/reference/api.md
Add how-to guide for AWS Spot instances and update API reference to document AWSSpotMarketOptions, maxPrice, semantics, and placement constraints.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant "K8s API Server" as API
  participant "HyperShift Operator\n(NodePool Controller)" as Controller
  participant "AWS EC2" as AWS

  User->>API: Create/Update NodePool (spec.platform.aws.placement.spotMarketOptions[, tenancy])
  API-->>User: CRD/CEL validation (reject if capacityReservation or tenancy invalid)

  rect rgba(220,235,255,0.4)
    note over API: Valid only if no capacityReservation and tenancy is unset or "default"
  end

  API->>Controller: Reconcile NodePool
  alt spotMarketOptions present
    Controller->>Controller: Validate tenancy & capacityReservation
    Controller->>Controller: Populate AWSMachineTemplate.Spec.SpotMarketOptions\nset MarketType=Spot, set MaxPrice if provided
  else no spotMarketOptions
    Controller->>Controller: Use On‑Demand configuration
  end
  Controller->>AWS: Create/Update Launch Template / Instances
  AWS-->>Controller: Provisioning result
  Controller-->>User: NodePool Ready / Nodes available
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Poem

I nibble code and patch the lair,
A max-price carrot floating there.
No hosts, no reservations — go!
Default burrows help things flow.
Hop—new Spot nodes rise with flair. 🥕🐇

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.2.2)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/product/migration-guide for migration instructions

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@openshift-ci openshift-ci Bot added do-not-merge/needs-area area/api Indicates the PR includes changes for the API area/documentation Indicates the PR includes changes for documentation labels Aug 27, 2025
@openshift-ci

openshift-ci Bot commented Aug 27, 2025

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gdbranco
Once this PR has been reviewed and has the lgtm label, please assign bryan-cox for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release and removed do-not-merge/needs-area labels Aug 27, 2025
Comment thread docs/content/how-to/aws/create-aws-hosted-cluster-spot-instances.md Outdated
- Ensure that the AWS service-linked role for Spot is enabled in the account where the hosted cluster will be installed. This is a one-time setup per account.
- You can verify if the role already exists using the following command:
```sh
aws iam get-role --role-name AWSServiceRoleForEC2Spot

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shoud be automated by the cli iam management

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR needs to add a new e2e for spot. Our e2e uses the same managed policies than rosa and let's you include additional perms

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rosa managed policies (worker account role) should suffice in terms of permissions for spot instances already. In regards to the grant due to KMS key I would need to double check, but given ROSA already offers the possibility of using custom KMS keys I expect the customer would need to handle the grant and include the worker role as a principal

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://cloud.redhat.com/experts/rosa/kms/

Example documentation capturing that


- Spot instances are subject to interruption when AWS needs the capacity back for On-Demand customers
- Instances receive a 2-minute warning before termination
- HyperShift automatically handles instance replacement when Spot instances are terminated

@enxebre enxebre Aug 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • HyperShift automatically handles instance replacement when Spot instances are terminated

can you articulate how? what happens when an instance is terminated with and without autorepair enabled and how is that failure bubble up?
we will also need to ship a termination handler for detecting upcoming interruptions and do a best effort to handle them gracefully

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that due to auto repair setting part of the node pool management when AWS terminates the spot instance it will cause a failure in the cluster infra which in turn will cause the node pool management to replace it trying to maintain the desired replica count. But for a more graceful approach it would indeed require more integration for detecting interruptions

Comment thread api/hypershift/v1beta1/aws.go Outdated
@enxebre

enxebre commented Aug 28, 2025

Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 28, 2025

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
hypershift-operator/controllers/nodepool/aws_test.go (1)

532-611: Add a test for Spot with non-default tenancy (should fail).

Complements the capacityReservation test and protects future regressions.

       testCases := []struct {
         name                 string
         hostedClusterVersion string
         setupNodePool        func() *hyperv1.NodePool
         oldCondition         *hyperv1.NodePoolCondition
         expectedError        string
       }{
+        {
+          name:                 "SpotMarketOptions with non-default tenancy should fail",
+          hostedClusterVersion: "4.19.0",
+          setupNodePool: func() *hyperv1.NodePool {
+            return &hyperv1.NodePool{
+              Spec: hyperv1.NodePoolSpec{
+                Platform: hyperv1.NodePoolPlatform{
+                  AWS: &hyperv1.AWSNodePoolPlatform{
+                    SpotMarketOptions: &hyperv1.AWSSpotMarketOptions{},
+                    Placement: &hyperv1.PlacementOptions{
+                      Tenancy: "dedicated",
+                    },
+                  },
+                },
+              },
+            }
+          },
+          expectedError: "spotMarketOptions require placement.tenancy to be 'default'",
+        },
♻️ Duplicate comments (4)
api/hypershift/v1beta1/aws.go (1)

61-65: Enforce mutual exclusivity (Spot vs CapacityReservation) and tenancy constraint at the API (CEL).

Controller-only validation is insufficient. Add type-level CEL to AWSNodePoolPlatform so invalid specs never persist. Also, Spot is not supported with dedicated/host tenancy; enforce tenancy=default when spotMarketOptions is set.

Add these validations at the AWSNodePoolPlatform type (type-level, not field-level):

- type AWSNodePoolPlatform struct {
+// +kubebuilder:validation:XValidation:rule="!(has(self.spotMarketOptions) && has(self.placement) && has(self.placement.capacityReservation))",message="spotMarketOptions cannot be used with placement.capacityReservation"
+// +kubebuilder:validation:XValidation:rule="!(has(self.spotMarketOptions) && has(self.placement) && has(self.placement.tenancy) && self.placement.tenancy != 'default')",message="spotMarketOptions require placement.tenancy to be 'default'"
+type AWSNodePoolPlatform struct {
docs/content/how-to/aws/create-aws-hosted-cluster-spot-instances.md (3)

1-108: Scope this under automated machine management and avoid re-documenting cluster creation.

Per prior feedback, fold this into the existing “Automated machine management” how-to, focusing on NodePool specifics; link to Getting Started for cluster creation.


14-21: CLI IAM step should be automated by the CLI IAM management.

Either remove or caveat this section; if kept, clearly state when it’s needed.


145-145: Clarify replacement behavior and termination handling.

CAPI/MachineDeployments will recreate instances to meet replicas, but graceful handling of 2‑minute interruptions needs a termination handler; don’t imply it “automatically handles” without detailing the mechanism.

- - HyperShift automatically handles instance replacement when Spot instances are terminated
+ - The control plane will recreate instances to maintain desired replicas when Spot instances are terminated. For graceful drains on the 2‑minute notice, deploy a termination handler DaemonSet (e.g., AWS Node Termination Handler).
🧹 Nitpick comments (4)
api/hypershift/v1beta1/aws.go (2)

67-74: Tighten MaxPrice validation and clarify units.

Cap the decimal precision and state “per instance-hour” explicitly.

- // The price is specified in USD. If omitted, the On-Demand price is used as the max price.
+ // The price is specified in USD per instance-hour. If omitted, the On-Demand price is used as the max price.
- // +kubebuilder:validation:Pattern=`^[0-9]+(\.[0-9]+)?$`
+ // +kubebuilder:validation:Pattern=`^[0-9]+(\.[0-9]{1,5})?$`

95-95: Typo in comment.

-// MarketType describes the market type of the CapacityReservationo for an Instance.
+// MarketType describes the market type of the CapacityReservation for an Instance.
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.go (1)

95-95: Mirror typo fix in vendor.

Keep comments consistent with the source API.

-// MarketType describes the market type of the CapacityReservationo for an Instance.
+// MarketType describes the market type of the CapacityReservation for an Instance.
docs/content/how-to/aws/create-aws-hosted-cluster-spot-instances.md (1)

70-77: Minor grammar tweak.

-When completed, extract the credentials for workload cluster:
+When complete, extract the workload cluster credentials:
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d219178 and 0f71a22.

📒 Files selected for processing (5)
  • api/hypershift/v1beta1/aws.go (1 hunks)
  • docs/content/how-to/aws/create-aws-hosted-cluster-spot-instances.md (1 hunks)
  • hypershift-operator/controllers/nodepool/aws.go (2 hunks)
  • hypershift-operator/controllers/nodepool/aws_test.go (3 hunks)
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.go (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/content/how-to/aws/create-aws-hosted-cluster-spot-instances.md

[style] ~12-~12: In American English, abbreviations like “etc.” require a period.
Context: ...(Pull Secret, Hosted Zone, OIDC Bucket, etc) - Ensure that the AWS service-linked r...

(ETC_PERIOD)


[grammar] ~70-~70: There might be a mistake here.
Context: ...When completed, extract the credentials for workload cluster: ```sh ./hypershift c...

(QB_NEW_EN)


[grammar] ~212-~212: There might be a mistake here.
Context: ...uptions: 1. Check the NodePool status: oc describe nodepool <nodepool-name> -n clusters 2. Verify that autoRepair is enabled in t...

(QB_NEW_EN)

🪛 markdownlint-cli2 (0.17.2)
docs/content/how-to/aws/create-aws-hosted-cluster-spot-instances.md

12-12: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


14-14: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


18-18: Code block style
Expected: fenced; Actual: indented

(MD046, code-block-style)

🔇 Additional comments (2)
hypershift-operator/controllers/nodepool/aws.go (1)

158-166: LGTM: correct propagation of Spot options and MarketType default.

Setting SpotMarketOptions and defaulting MarketType=Spot only when unset is correct.

hypershift-operator/controllers/nodepool/aws_test.go (1)

169-194: Spot propagation tests are solid.

Covers MaxPrice and defaulting MarketType=Spot.

Comment thread docs/content/how-to/aws/create-aws-hosted-cluster-spot-instances.md Outdated
Comment thread docs/content/how-to/aws/create-aws-hosted-cluster-spot-instances.md Outdated
Comment thread hypershift-operator/controllers/nodepool/aws.go Outdated
@gdbranco
gdbranco force-pushed the feat/ocpstrat-1677 branch from 0f71a22 to c4b4d87 Compare August 29, 2025 16:58
@openshift-ci openshift-ci Bot added the area/cli Indicates the PR includes changes for CLI label Aug 29, 2025
@cwbotbot

cwbotbot commented Aug 29, 2025

Copy link
Copy Markdown

Test Results

e2e-aws

e2e-aks

@gdbranco
gdbranco force-pushed the feat/ocpstrat-1677 branch 3 times, most recently from 284e138 to 633d511 Compare September 2, 2025 13:28
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2025
@gdbranco
gdbranco force-pushed the feat/ocpstrat-1677 branch 2 times, most recently from e4eb40a to b767691 Compare September 2, 2025 14:01
@openshift-ci openshift-ci Bot added the area/testing Indicates the PR includes changes for e2e testing label Sep 2, 2025
@gdbranco
gdbranco force-pushed the feat/ocpstrat-1677 branch 2 times, most recently from 7f67ba5 to 8f5dede Compare September 2, 2025 16:51

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (3)
docs/content/how-to/automated-machine-management/aws-spot-instances.md (2)

14-21: Fix list/code block indentation (markdownlint MD007/MD046).

Bullets under prerequisites are over-indented; fenced blocks should be nested under the bullet with two-space indent.

Apply:

-    - You can verify if the role already exists using the following command:
-  ```sh
-  aws iam get-role --role-name AWSServiceRoleForEC2Spot
-  ```
-    - If the role does not exist, create it with:
-  ```sh
-  aws iam create-service-linked-role --aws-service-name spot.amazonaws.com
-  ```
+  - You can verify if the role already exists using:
+    ```sh
+    aws iam get-role --role-name AWSServiceRoleForEC2Spot
+    ```
+  - If the role does not exist, create it with:
+    ```sh
+    aws iam create-service-linked-role --aws-service-name spot.amazonaws.com
+    ```

94-94: Document the exact maxPrice format to match the CRD.

Align text with validation: digits/precision and no leading zeros (except "0"). Include valid/invalid examples.

Apply:

-- **maxPrice** (optional): The maximum price per hour that you're willing to pay for a Spot instance, specified in USD. If omitted, the On-Demand price is used as the maximum price.
+- **maxPrice** (optional): The maximum price per hour (USD). If omitted, the On-Demand price is used. Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless the value is exactly "0"; scientific notation not allowed. Examples: "0", "0.0739", "1234567890.123456". Invalid: "00.10", "1.", ".5", "1.1234567".
-3. Check that your `maxPrice` format is valid (numeric string in USD)
+3. Check that your `maxPrice` format is valid (numeric string in USD; up to 10 integer digits and 6 fractional digits; no leading zeros unless "0")

Also applies to: 205-208

cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml (1)

528-533: CEL rule/message alignment resolved as previously requested.

Message now states “‘default’ or unset,” matching the rule. Good catch and fix.

🧹 Nitpick comments (6)
test/e2e/create_cluster_test.go (1)

1795-1865: Deduplicate the expected error string to avoid drift.

Define a local constant and reuse it across these cases to keep tests resilient to future message tweaks.

Add near the top of this test file:

const spotError = "spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')"

Then replace the repeated string literals in these cases with spotError.

hypershift-operator/controllers/nodepool/aws_test.go (2)

543-666: Factor out the unified error message literal.

Use a shared constant in this file to avoid message drift and copy-paste.

Example:

const spotErr = "spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')"

Replace the repeated literals in failing cases with spotErr.


543-666: Optional: add a MaxPrice format rejection test.

Consider a controller-level test asserting that an invalid maxPrice (e.g., "00.10", ".5", "1.1234567") surfaces the API validation error, complementing CRD/e2e coverage.

api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yaml (1)

493-501: Spot options schema and price regex look good; tweak dashes in docs.

Schema/regex are correct and bounded (maxLength 17 aligns with 10+1+6). Minor nit: replace typographic dashes with ASCII hyphens to avoid encoding surprises in generators/tooling.

-                                  maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
-                                  If omitted, the On‑Demand price is used as the ceiling.
+                                  maxPrice defines the maximum price (USD per instance-hour) you are willing to pay for a Spot instance.
+                                  If omitted, the On-Demand price is used as the ceiling.
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml (1)

496-504: Spot options schema: solid bounds; unify hyphens in description.

Validation matches intended format; minor doc nit on typographic dashes.

-                                  maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
-                                  If omitted, the On‑Demand price is used as the ceiling.
+                                  maxPrice defines the maximum price (USD per instance-hour) you are willing to pay for a Spot instance.
+                                  If omitted, the On-Demand price is used as the ceiling.
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml (1)

496-504: Spot options schema OK; normalize hyphens in docs.

Same nit as other CRDs: prefer ASCII hyphens.

-                                  maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
-                                  If omitted, the On‑Demand price is used as the ceiling.
+                                  maxPrice defines the maximum price (USD per instance-hour) you are willing to pay for a Spot instance.
+                                  If omitted, the On-Demand price is used as the ceiling.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 9c13e31 and 2074461.

📒 Files selected for processing (20)
  • api/hypershift/v1beta1/aws.go (2 hunks)
  • api/hypershift/v1beta1/zz_generated.deepcopy.go (2 hunks)
  • api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yaml (2 hunks)
  • api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yaml (2 hunks)
  • client/applyconfiguration/hypershift/v1beta1/awsnodepoolplatform.go (2 hunks)
  • client/applyconfiguration/hypershift/v1beta1/awsspotmarketoptions.go (1 hunks)
  • client/applyconfiguration/hypershift/v1beta1/placementoptions.go (2 hunks)
  • client/applyconfiguration/utils.go (1 hunks)
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml (2 hunks)
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml (2 hunks)
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml (2 hunks)
  • docs/content/how-to/automated-machine-management/aws-spot-instances.md (1 hunks)
  • docs/content/reference/api.md (2 hunks)
  • hypershift-operator/controllers/nodepool/aws.go (4 hunks)
  • hypershift-operator/controllers/nodepool/aws_test.go (4 hunks)
  • test/e2e/create_cluster_test.go (1 hunks)
  • test/e2e/nodepool_spotmarketoptions_test.go (1 hunks)
  • test/e2e/nodepool_test.go (1 hunks)
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.go (2 hunks)
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • client/applyconfiguration/hypershift/v1beta1/awsnodepoolplatform.go
🚧 Files skipped from review as they are similar to previous changes (12)
  • api/hypershift/v1beta1/zz_generated.deepcopy.go
  • test/e2e/nodepool_spotmarketoptions_test.go
  • hypershift-operator/controllers/nodepool/aws.go
  • client/applyconfiguration/utils.go
  • client/applyconfiguration/hypershift/v1beta1/placementoptions.go
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go
  • client/applyconfiguration/hypershift/v1beta1/awsspotmarketoptions.go
  • docs/content/reference/api.md
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.go
  • api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yaml
  • test/e2e/nodepool_test.go
  • api/hypershift/v1beta1/aws.go
🧰 Additional context used
📓 Path-based instructions (9)
**/*.md

📄 CodeRabbit inference engine (.cursor/rules/code-formatting.mdc)

For markdown files, use make verify-codespell to catch spelling errors

Files:

  • docs/content/how-to/automated-machine-management/aws-spot-instances.md
**/!(*.pb).go

📄 CodeRabbit inference engine (.cursor/rules/100-go-mistakes.mdc)

**/!(*.pb).go: Avoid variable shadowing
Do not over-nest control flow (e.g., nested if or for blocks)
Avoid init() functions unless absolutely necessary
Keep functions small and focused
Prefer composition over inheritance (via embedding)
Use the functional options pattern for constructors where flexibility is needed
Avoid defining interfaces until you need them
Do not return interfaces from constructors or public APIs
Define interfaces on the consumer side, not the producer side
Keep interfaces small and focused (generally 1–2 methods)
Avoid embedding pointer types unless necessary
Don’t overuse getters/setters — prefer public fields when it makes sense
Use value receivers when the method doesn't mutate state or require pointer semantics
Do not use util, common, or similarly vague package names
Avoid package name collisions by using clear, unique names
Do not expose unnecessary symbols (keep exported API minimal)
Distinguish between nil and empty slices
Avoid memory leaks from slicing large arrays
Always check the capacity when copying or appending slices
Preallocate slice capacity when size is known ahead of time
Always initialize maps before use
Check existence with the two-value assignment (val, ok := m[key])
Be aware that ranging over a map is in random order
Always check errors — don’t ignore them
Wrap errors with context when rethrowing
Avoid panics except in truly unrecoverable cases
Use errors.Is and errors.As for error comparison in Go 1.20+
Always defer cancel() when using context.WithCancel
Do not leak goroutines — ensure they exit cleanly
Avoid data races — use mutexes or channels appropriately
Never close a channel from the receiving side
Keep imports grouped and ordered: stdlib, external, internal
Avoid magic numbers — use named constants
Prefer explicit over implicit — especially in exported APIs
Only use generics when they simplify code or add real flexibility
Avoid over-engineering with type parameters
Be cautious with constraint complexity — keep...

Files:

  • hypershift-operator/controllers/nodepool/aws_test.go
  • test/e2e/create_cluster_test.go
**/*_test.go

📄 CodeRabbit inference engine (.cursor/rules/100-go-mistakes.mdc)

**/*_test.go: Name tests consistently: TestXxx, BenchmarkXxx, ExampleXxx
Use table-driven tests where possible
Avoid global state in tests
Use t.Helper() in helper functions to improve error tracebacks

Always use "When ... it should ..." format for describing test cases when creating unit tests

Files:

  • hypershift-operator/controllers/nodepool/aws_test.go
  • test/e2e/create_cluster_test.go
**/*.go

📄 CodeRabbit inference engine (.cursor/rules/code-formatting.mdc)

Use make lint-fix after writing Go code to automatically fix most linting issues

Follow the rules defined in @100-go-mistakes.mdc for Go code

Files:

  • hypershift-operator/controllers/nodepool/aws_test.go
  • test/e2e/create_cluster_test.go
hypershift-operator/controllers/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Place operator controller implementations under hypershift-operator/controllers/

Files:

  • hypershift-operator/controllers/nodepool/aws_test.go
{hypershift-operator,control-plane-operator}/controllers/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

{hypershift-operator,control-plane-operator}/controllers/**/*.go: Controller code should follow controller-runtime patterns with proper error handling and requeuing
Use controller-runtime structured logging in controllers

Files:

  • hypershift-operator/controllers/nodepool/aws_test.go
{hypershift-operator,control-plane-operator}/controllers/**

📄 CodeRabbit inference engine (AGENTS.md)

Place platform-specific implementations within their respective controller subdirectories to keep platform logic isolated

Files:

  • hypershift-operator/controllers/nodepool/aws_test.go
test/e2e/**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

Place end-to-end (E2E) tests under test/e2e/

Files:

  • test/e2e/create_cluster_test.go
api/**

📄 CodeRabbit inference engine (AGENTS.md)

api/**: API definitions and CRDs must reside under the api/ directory
Prefer API version v1beta1; use feature gates for experimental functionality
Generate CRDs via controller-gen using the OpenShift-specific tooling for this repository

Files:

  • api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yaml
🧬 Code graph analysis (2)
hypershift-operator/controllers/nodepool/aws_test.go (5)
api/hypershift/v1beta1/aws.go (4)
  • PlacementOptions (77-105)
  • AWSSpotMarketOptions (63-72)
  • MarketType (108-108)
  • CapacityReservationOptions (121-159)
client/applyconfiguration/hypershift/v1beta1/placementoptions.go (1)
  • PlacementOptions (30-32)
client/applyconfiguration/hypershift/v1beta1/awsspotmarketoptions.go (1)
  • AWSSpotMarketOptions (28-30)
api/vendor/k8s.io/utils/ptr/ptr.go (1)
  • To (50-52)
vendor/github.com/aws/aws-sdk-go/service/ec2/api.go (3)
  • CapacityReservation (66652-66785)
  • TenancyDedicated (202018-202018)
  • TenancyDefault (202015-202015)
test/e2e/create_cluster_test.go (2)
api/hypershift/v1beta1/aws.go (4)
  • PlacementOptions (77-105)
  • AWSSpotMarketOptions (63-72)
  • CapacityReservationOptions (121-159)
  • MarketType (108-108)
api/vendor/k8s.io/utils/ptr/ptr.go (1)
  • To (50-52)
🪛 LanguageTool
docs/content/how-to/automated-machine-management/aws-spot-instances.md

[grammar] ~11-~11: There might be a mistake here.
Context: ...xisting HyperShift hosted cluster on AWS - Access to the management cluster where t...

(QB_NEW_EN)


[grammar] ~12-~12: There might be a mistake here.
Context: ...where the NodePool resources are created - Ensure that the AWS service-linked role ...

(QB_NEW_EN)


[grammar] ~121-~121: There might be a mistake here.
Context: ...inated: 1. The Kubernetes node becomes NotReady 2. The NodePool controller detects the fail...

(QB_NEW_EN)


[style] ~124-~124: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ... is automatically requested 4. Pods are automatically rescheduled to available nodes ## Best...

(ADVERB_REPETITION_PREMIUM)


[grammar] ~128-~128: There might be a mistake here.
Context: ...ilable nodes ## Best practices ### 1. Set appropriate max price Consider setting...

(QB_NEW_EN)

🪛 markdownlint-cli2 (0.17.2)
docs/content/how-to/automated-machine-management/aws-spot-instances.md

14-14: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


18-18: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

🔇 Additional comments (6)
test/e2e/create_cluster_test.go (1)

1795-1865: Spot placement validation cases are correct and align with CRD/CEL message.

The five new negative tests accurately exercise tenancy and capacity reservation incompatibilities for Spot; the expected error substring matches the CRD message.

hypershift-operator/controllers/nodepool/aws_test.go (3)

15-16: OK to use AWS SDK tenancy constants in tests.

Importing ec2 for Tenancy constants improves clarity and reduces typos.


171-200: Correct mapping to CAPA: SpotMarketOptions and MarketType=Spot.

Both cases validate the translation of NodePool spot options into AWSMachineTemplate (including MarketTypeSpot). Good coverage.


543-666: Validation matrix for Spot vs tenancy/capacityReservation looks solid.

Cases cover <4.19 gating for capacityReservation and Spot incompatibilities; messages match the unified CEL/controller string.

cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml (1)

528-533: CEL rule/message matches tests and docs. LGTM.

Cross-field validation for Spot vs capacityReservation/tenancy is correct and consistent.

cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml (1)

528-533: Placement CEL validation is correct and consistent.

Mutual exclusion with capacityReservation and tenancy constraint are properly enforced; message matches rule.

Comment on lines +490 to +505
spotMarketOptions:
description: |-
spotMarketOptions specifies options for using AWS Spot instances.
When specified, instances will be launched as Spot instances with the given configuration.
Mutually exclusive with capacityReservation, and tenancy must be unset or set to "default".
properties:
maxPrice:
description: |-
maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
If omitted, the On‑Demand price is used as the ceiling.
Example: "0.0739"
Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless the value is "0"; scientific notation is not allowed.
maxLength: 17
pattern: ^(0|[1-9][0-9]{0,9})(\.[0-9]{1,6})?$
type: string
type: object

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Non-ASCII hyphens in descriptions (“instance‑hour”, “On‑Demand”).

Replace U+2011/U+2013 style hyphens with ASCII "-" in the Go API comments (api/hypershift/v1beta1/aws.go), then regenerate CRDs to avoid encoding issues in tooling.

Apply in api/hypershift/v1beta1/aws.go:

-// maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
-// If omitted, the On‑Demand price is used as the ceiling.
+// maxPrice defines the maximum price (USD per instance-hour) you are willing to pay for a Spot instance.
+// If omitted, the On-Demand price is used as the ceiling.

Then run codegen and verify no non-ASCII hyphens remain:


🏁 Script executed:

#!/bin/bash
rg -nU "[\u2010-\u2015]" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests || true

Length of output: 1397


Fix non-ASCII hyphens and regenerate CRDs

The following generated CRDs still contain non-ASCII hyphens at lines 498–499:

  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml

Apply the diff in api/hypershift/v1beta1/aws.go, rerun codegen, then verify no non-ASCII hyphens remain:

rg -nU "[\u2010-\u2015]" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests || true
🤖 Prompt for AI Agents
In
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml
around lines 490–505 there are non-ASCII hyphens in the generated CRD
descriptions (lines ~498–499); to fix, update api/hypershift/v1beta1/aws.go to
replace any Unicode hyphen/dash characters with the ASCII hyphen-minus, save,
re-run the CRD code generation pipeline (codegen), and then verify no non-ASCII
hyphens remain by running the suggested ripgrep check (rg -nU "[\u2010-\u2015]"
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests || true);
commit the updated source file and the regenerated CRD manifests.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (9)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yaml (1)

525-530: Ensure the CEL rule/message appears in every NodePool CRD variant

This rule/message is correct. Please verify it’s present across all generated NodePool CRDs (operator and feature‑gated) to avoid drift between variants.

Run:

#!/bin/bash
set -euo pipefail
pat_rule="has\\(self\\.spotMarketOptions\\) \\? \\(!has\\(self\\.capacityReservation\\) && \\(!has\\(self\\.tenancy\\) \\|\\| self\\.tenancy == 'default'\\)\\) : true"
pat_msg="spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')"
for d in cmd/install/assets/hypershift-operator/zz_generated.crd-manifests api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests; do
  echo "Scanning $d"
  mapfile -t files < <(fd -t f -g 'nodepools*.yaml' "$d")
  for f in "${files[@]}"; do
    have_rule=$(rg -n "$pat_rule" "$f" || true)
    have_msg=$(rg -n "$pat_msg" "$f" || true)
    [[ -z "$have_rule" || -z "$have_msg" ]] && echo "MISSING in $f"
  done
done
api/hypershift/v1beta1/aws.go (1)

62-71: Confirm MaxPrice validation parity with any vendored/api duplicates

The stricter Pattern/MaxLength are fine, but please ensure any duplicate AWSSpotMarketOptions definitions (e.g., vendor copies) use the same annotations to prevent CRD/codegen drift.

Run:

#!/bin/bash
set -euo pipefail
echo "Definitions of AWSSpotMarketOptions across repo:"
rg -nPU '(?s)^type\s+AWSSpotMarketOptions\b.*?\n}' -g '**/*.go'
echo
echo "MaxPrice validation tags:"
rg -n 'MaxPrice.*kubebuilder:validation' -g '**/*.go'
hypershift-operator/controllers/nodepool/aws.go (1)

169-175: Clear CapacityReservation fields when switching to Spot.

If Spot is set, carry-over CapacityReservation* fields from earlier placement processing can leak into the template. Clear them to avoid emitting mutually exclusive settings, even if CRD validation normally blocks such combos.

Apply:

 if nodePool.Spec.Platform.AWS.Placement != nil && nodePool.Spec.Platform.AWS.Placement.SpotMarketOptions != nil {
+    // Clear mutually exclusive CapacityReservation settings when using Spot.
+    awsMachineTemplateSpec.Template.Spec.CapacityReservationID = nil
+    awsMachineTemplateSpec.Template.Spec.CapacityReservationPreference = ""
     awsMachineTemplateSpec.Template.Spec.SpotMarketOptions = &capiaws.SpotMarketOptions{
         MaxPrice: nodePool.Spec.Platform.AWS.Placement.SpotMarketOptions.MaxPrice,
     }
     awsMachineTemplateSpec.Template.Spec.MarketType = capiaws.MarketTypeSpot
 }

Would you like a unit test ensuring these fields are nil/empty when SpotMarketOptions is set?

docs/content/how-to/automated-machine-management/aws-spot-instances.md (3)

14-21: Fix nested list indentation under Prerequisites (MD007/MD046).

Indent list items with two spaces and fence code blocks under the bullet (4 spaces).

-    - You can verify if the role already exists using the following command:
-  ```sh
-  aws iam get-role --role-name AWSServiceRoleForEC2Spot
-  ```
-    - If the role does not exist, create it with:
-  ```sh
-  aws iam create-service-linked-role --aws-service-name spot.amazonaws.com
-  ```
+  - You can verify if the role already exists using the following command:
+    ```sh
+    aws iam get-role --role-name AWSServiceRoleForEC2Spot
+    ```
+  - If the role does not exist, create it with:
+    ```sh
+    aws iam create-service-linked-role --aws-service-name spot.amazonaws.com
+    ```

94-94: Document exact maxPrice format to match CRD validation.

State digits/precision and give valid/invalid examples to avoid user errors.

-- **maxPrice** (optional): The maximum price per hour that you're willing to pay for a Spot instance, specified in USD. If omitted, the On-Demand price is used as the maximum price.
+- **maxPrice** (optional): Maximum price per hour (USD). If omitted, the On-Demand price is used. Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless exactly "0"; scientific notation not allowed. Examples: "0", "0.0739", "1234567890.123456". Invalid: "00.10", "1.", ".5", "1.1234567".

205-207: Align validation step with CRD maxPrice constraints.

Mirror the precise format constraints users must follow.

-3. Check that your `maxPrice` format is valid (numeric string in USD)
+3. Check that your `maxPrice` format is valid (numeric string in USD; up to 10 integer digits and up to 6 fractional digits; no leading zeros unless "0")
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml (1)

498-501: Replace non-ASCII hyphens in generated descriptions and regenerate CRDs.

“instance‑hour” and “On‑Demand” contain Unicode hyphens; switch to ASCII “-” in Go comments and re-run codegen.

-  maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
-  If omitted, the On‑Demand price is used as the ceiling.
+  maxPrice defines the maximum price (USD per instance-hour) you are willing to pay for a Spot instance.
+  If omitted, the On-Demand price is used as the ceiling.
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml (1)

498-501: Fix Unicode hyphens in descriptions and regenerate CRDs.

Same non-ASCII hyphen issue here; replace in source comments and regenerate.

-  maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
-  If omitted, the On‑Demand price is used as the ceiling.
+  maxPrice defines the maximum price (USD per instance-hour) you are willing to pay for a Spot instance.
+  If omitted, the On-Demand price is used as the ceiling.
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml (1)

528-533: CEL rule/message alignment for Spot constraints looks good

Message now matches the rule (“'default' or unset”) and enforces incompatibility with capacityReservation. LGTM.

Run this quick check to ensure all NodePool CRDs carry the Spot field and the aligned validation:

#!/bin/bash
set -euo pipefail
echo "Scanning NodePool CRDs for spotMarketOptions and aligned validation..."
for f in $(fd -t f -g 'nodepools-*.crd.yaml' cmd/install/assets/hypershift-operator/zz_generated.crd-manifests); do
  echo "=== $f"
  rg -nC1 '\bspotMarketOptions\b' "$f" || echo "MISSING spotMarketOptions in $f"
  rg -nC1 "requires tenancy to be 'default' or unset" "$f" || echo "MISSING aligned message in $f"
  rg -nC1 "has\\(self\\.spotMarketOptions\\).*!has\\(self\\.capacityReservation\\).*self\\.tenancy == 'default'" "$f" || echo "MISSING CEL rule in $f"
done
🧹 Nitpick comments (5)
test/e2e/create_cluster_test.go (1)

1795-1865: The repeated error string is already covered by a success case in test/e2e/nodepool_test.go, so you can drop the optional “add success-path” suggestion. Retain the nit: factor out the error message. Rewrite to:

Factor out repeated error message into a constant
Define at the top of this test block:

const spotIncompatibilityErr = "spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')"

Then replace each expectedErrorSubstring: "…" with

expectedErrorSubstring: spotIncompatibilityErr,
hypershift-operator/controllers/nodepool/aws.go (1)

312-314: Refine error for better operator diagnostics.

Returning a generic error is fine; consider including the NodePool name for context.

-    return fmt.Errorf("aws platform not populated")
+    return fmt.Errorf("aws platform not populated for NodePool %q", nodePool.Name)
docs/content/reference/api.md (2)

2533-2565: Spot options doc is clear; minor wording and typographic nits.

  • Use normal hyphens instead of non-breaking ones in “instance‑hour” and “On‑Demand”.
  • Consider clarifying accepted examples and keeping wording tight.

Apply this diff to tighten wording and fix hyphens:

-<p>maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
-If omitted, the On‑Demand price is used as the ceiling.
-Example: “0.0739”
-Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless the value is “0”; scientific notation is not allowed.</p>
+<p>maxPrice defines the maximum price (USD per instance-hour) you are willing to pay for a Spot instance.
+If omitted, the On-Demand price is used as the ceiling.
+Example: “0.0739”.
+Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless the value is “0”; scientific notation is not allowed.</p>

10472-10487: Tighten exclusivity wording for Spot with tenancy/capacity reservations.

Clarify that Spot cannot be combined with dedicated/host tenancy and is exclusive with capacity reservations.

-<p>spotMarketOptions specifies options for using AWS Spot instances.
-When specified, instances will be launched as Spot instances with the given configuration.
-Mutually exclusive with capacityReservation, and tenancy must be unset or set to “default”.</p>
+<p>spotMarketOptions specifies options for using AWS Spot instances.
+When specified, instances will be launched as Spot instances with the given configuration.
+Mutually exclusive with <code>capacityReservation</code>; <code>tenancy</code> must be omitted or set to “default” (Spot is not supported with “dedicated” or “host” tenancy).</p>
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml (1)

490-505: Use ASCII hyphens in descriptions to avoid encoding/rendering glitches

Replace the non-breaking hyphen characters in "instance‑hour" and "On‑Demand" with regular ASCII hyphens.

-                                  maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
+                                  maxPrice defines the maximum price (USD per instance-hour) you are willing to pay for a Spot instance.
-                                  If omitted, the On‑Demand price is used as the ceiling.
+                                  If omitted, the On-Demand price is used as the ceiling.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2074461 and 990dbe8.

📒 Files selected for processing (19)
  • api/hypershift/v1beta1/aws.go (2 hunks)
  • api/hypershift/v1beta1/zz_generated.deepcopy.go (2 hunks)
  • api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yaml (2 hunks)
  • api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yaml (2 hunks)
  • client/applyconfiguration/hypershift/v1beta1/awsspotmarketoptions.go (1 hunks)
  • client/applyconfiguration/hypershift/v1beta1/placementoptions.go (2 hunks)
  • client/applyconfiguration/utils.go (1 hunks)
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml (2 hunks)
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml (2 hunks)
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml (2 hunks)
  • docs/content/how-to/automated-machine-management/aws-spot-instances.md (1 hunks)
  • docs/content/reference/api.md (2 hunks)
  • hypershift-operator/controllers/nodepool/aws.go (4 hunks)
  • hypershift-operator/controllers/nodepool/aws_test.go (4 hunks)
  • test/e2e/create_cluster_test.go (1 hunks)
  • test/e2e/nodepool_spotmarketoptions_test.go (1 hunks)
  • test/e2e/nodepool_test.go (1 hunks)
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.go (2 hunks)
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
  • client/applyconfiguration/hypershift/v1beta1/placementoptions.go
  • client/applyconfiguration/utils.go
  • test/e2e/nodepool_test.go
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.go
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go
  • client/applyconfiguration/hypershift/v1beta1/awsspotmarketoptions.go
  • test/e2e/nodepool_spotmarketoptions_test.go
  • api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yaml
  • hypershift-operator/controllers/nodepool/aws_test.go
  • api/hypershift/v1beta1/zz_generated.deepcopy.go
🧰 Additional context used
📓 Path-based instructions (10)
**/!(*.pb).go

📄 CodeRabbit inference engine (.cursor/rules/100-go-mistakes.mdc)

**/!(*.pb).go: Avoid variable shadowing
Do not over-nest control flow (e.g., nested if or for blocks)
Avoid init() functions unless absolutely necessary
Keep functions small and focused
Prefer composition over inheritance (via embedding)
Use the functional options pattern for constructors where flexibility is needed
Avoid defining interfaces until you need them
Do not return interfaces from constructors or public APIs
Define interfaces on the consumer side, not the producer side
Keep interfaces small and focused (generally 1–2 methods)
Avoid embedding pointer types unless necessary
Don’t overuse getters/setters — prefer public fields when it makes sense
Use value receivers when the method doesn't mutate state or require pointer semantics
Do not use util, common, or similarly vague package names
Avoid package name collisions by using clear, unique names
Do not expose unnecessary symbols (keep exported API minimal)
Distinguish between nil and empty slices
Avoid memory leaks from slicing large arrays
Always check the capacity when copying or appending slices
Preallocate slice capacity when size is known ahead of time
Always initialize maps before use
Check existence with the two-value assignment (val, ok := m[key])
Be aware that ranging over a map is in random order
Always check errors — don’t ignore them
Wrap errors with context when rethrowing
Avoid panics except in truly unrecoverable cases
Use errors.Is and errors.As for error comparison in Go 1.20+
Always defer cancel() when using context.WithCancel
Do not leak goroutines — ensure they exit cleanly
Avoid data races — use mutexes or channels appropriately
Never close a channel from the receiving side
Keep imports grouped and ordered: stdlib, external, internal
Avoid magic numbers — use named constants
Prefer explicit over implicit — especially in exported APIs
Only use generics when they simplify code or add real flexibility
Avoid over-engineering with type parameters
Be cautious with constraint complexity — keep...

Files:

  • api/hypershift/v1beta1/aws.go
  • test/e2e/create_cluster_test.go
  • hypershift-operator/controllers/nodepool/aws.go
**/*.go

📄 CodeRabbit inference engine (.cursor/rules/code-formatting.mdc)

Use make lint-fix after writing Go code to automatically fix most linting issues

Follow the rules defined in @100-go-mistakes.mdc for Go code

Files:

  • api/hypershift/v1beta1/aws.go
  • test/e2e/create_cluster_test.go
  • hypershift-operator/controllers/nodepool/aws.go
api/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

After modifying API types in the api/ package, run make api to regenerate APIs and CRDs

Files:

  • api/hypershift/v1beta1/aws.go
api/**

📄 CodeRabbit inference engine (AGENTS.md)

api/**: API definitions and CRDs must reside under the api/ directory
Prefer API version v1beta1; use feature gates for experimental functionality
Generate CRDs via controller-gen using the OpenShift-specific tooling for this repository

Files:

  • api/hypershift/v1beta1/aws.go
  • api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yaml
**/*_test.go

📄 CodeRabbit inference engine (.cursor/rules/100-go-mistakes.mdc)

**/*_test.go: Name tests consistently: TestXxx, BenchmarkXxx, ExampleXxx
Use table-driven tests where possible
Avoid global state in tests
Use t.Helper() in helper functions to improve error tracebacks

Always use "When ... it should ..." format for describing test cases when creating unit tests

Files:

  • test/e2e/create_cluster_test.go
test/e2e/**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

Place end-to-end (E2E) tests under test/e2e/

Files:

  • test/e2e/create_cluster_test.go
**/*.md

📄 CodeRabbit inference engine (.cursor/rules/code-formatting.mdc)

For markdown files, use make verify-codespell to catch spelling errors

Files:

  • docs/content/reference/api.md
  • docs/content/how-to/automated-machine-management/aws-spot-instances.md
hypershift-operator/controllers/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Place operator controller implementations under hypershift-operator/controllers/

Files:

  • hypershift-operator/controllers/nodepool/aws.go
{hypershift-operator,control-plane-operator}/controllers/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

{hypershift-operator,control-plane-operator}/controllers/**/*.go: Controller code should follow controller-runtime patterns with proper error handling and requeuing
Use controller-runtime structured logging in controllers

Files:

  • hypershift-operator/controllers/nodepool/aws.go
{hypershift-operator,control-plane-operator}/controllers/**

📄 CodeRabbit inference engine (AGENTS.md)

Place platform-specific implementations within their respective controller subdirectories to keep platform logic isolated

Files:

  • hypershift-operator/controllers/nodepool/aws.go
🧠 Learnings (1)
📚 Learning: 2025-08-28T17:26:17.059Z
Learnt from: maxcao13
PR: openshift/hypershift#0
File: :0-0
Timestamp: 2025-08-28T17:26:17.059Z
Learning: The Karpenter provisioner check pattern with potential nil pointer dereference appears in 4 locations in the hypershift codebase: test/e2e/util/util.go, hypershift-operator/controllers/nodepool/token.go, hypershift-operator/controllers/hostedcluster/karpenter.go, and control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go. A 5th location in control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/component.go already implements proper nil checks.

Applied to files:

  • hypershift-operator/controllers/nodepool/aws.go
🧬 Code graph analysis (3)
api/hypershift/v1beta1/aws.go (1)
client/applyconfiguration/hypershift/v1beta1/awsspotmarketoptions.go (1)
  • AWSSpotMarketOptions (28-30)
test/e2e/create_cluster_test.go (2)
api/hypershift/v1beta1/aws.go (4)
  • PlacementOptions (77-105)
  • AWSSpotMarketOptions (63-72)
  • CapacityReservationOptions (121-159)
  • MarketType (108-108)
api/vendor/k8s.io/utils/ptr/ptr.go (1)
  • To (50-52)
hypershift-operator/controllers/nodepool/aws.go (2)
api/hypershift/v1beta1/aws.go (1)
  • MarketType (108-108)
vendor/github.com/aws/aws-sdk-go/service/ec2/api.go (2)
  • TenancyDefault (202015-202015)
  • CapacityReservation (66652-66785)
🪛 LanguageTool
docs/content/how-to/automated-machine-management/aws-spot-instances.md

[grammar] ~11-~11: There might be a mistake here.
Context: ...xisting HyperShift hosted cluster on AWS - Access to the management cluster where t...

(QB_NEW_EN)


[grammar] ~12-~12: There might be a mistake here.
Context: ...where the NodePool resources are created - Ensure that the AWS service-linked role ...

(QB_NEW_EN)


[grammar] ~121-~121: There might be a mistake here.
Context: ...inated: 1. The Kubernetes node becomes NotReady 2. The NodePool controller detects the fail...

(QB_NEW_EN)


[style] ~124-~124: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ... is automatically requested 4. Pods are automatically rescheduled to available nodes ## Best...

(ADVERB_REPETITION_PREMIUM)


[grammar] ~128-~128: There might be a mistake here.
Context: ...ilable nodes ## Best practices ### 1. Set appropriate max price Consider setting...

(QB_NEW_EN)

🪛 markdownlint-cli2 (0.17.2)
docs/content/how-to/automated-machine-management/aws-spot-instances.md

14-14: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


18-18: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
🔇 Additional comments (7)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yaml (1)

487-502: spotMarketOptions schema looks good; regex/length match the documented format

Pattern and MaxLength align with the described constraints and common AWS Spot expectations. No changes needed here.

api/hypershift/v1beta1/aws.go (2)

74-77: CEL cross-field validation for Spot incompatibilities is correct

Rule precisely enforces “no capacityReservation” and tenancy unset/default when spotMarketOptions is set. Message matches tests. LGTM.


100-104: Doc comment matches validation; field placement under PlacementOptions is appropriate

The comment reflects the enforced constraints and the optionality is correct. No changes needed.

hypershift-operator/controllers/nodepool/aws.go (1)

10-11: Good use of SDK constant for tenancy.

Importing ec2 to use TenancyDefault avoids magic strings and keeps validation aligned with AWS semantics.

docs/content/reference/api.md (1)

1-6: Run codespell for markdown docs.

Since this is a markdown doc, please run “make verify-codespell” to catch any stray typos introduced alongside the new sections.

cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml (1)

528-533: LGTM: Cross-field validation correctly enforces Spot incompatibilities.

Rule cleanly forbids capacityReservation with Spot and restricts tenancy to default/unset.

cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml (1)

528-533: LGTM: Cross-field validation mirrors Default CRD and is correct.

Consistent enforcement across feature sets.

Comment on lines +1 to +221
---
title: Manage AWS Spot instances in NodePools
---

AWS Spot instances allow you to run node pools using spare EC2 capacity at significantly reduced costs compared to On-Demand pricing. HyperShift supports configuring node pools to use AWS Spot instances through the `spotMarketOptions` field in the node pool specification.

This guide demonstrates how to configure and manage NodePools with AWS Spot instances in an existing hosted cluster.

## Prerequisites

- An existing HyperShift hosted cluster on AWS
- Access to the management cluster where the NodePool resources are created
- Ensure that the AWS service-linked role for Spot is enabled in the account where the hosted cluster is installed. This is a one-time setup per account.
- You can verify if the role already exists using the following command:
```sh
aws iam get-role --role-name AWSServiceRoleForEC2Spot
```
- If the role does not exist, create it with:
```sh
aws iam create-service-linked-role --aws-service-name spot.amazonaws.com
```

## Configure NodePool with Spot instances

### Option 1: Create a new NodePool with Spot instances

Create a new NodePool configured to use AWS Spot instances:

```sh
cat << EOF | oc apply -f -
apiVersion: hypershift.openshift.io/v1beta1
kind: NodePool
metadata:
name: spot-nodepool
namespace: clusters
spec:
clusterName: <your-cluster-name>
replicas: 2
management:
autoRepair: true
upgradeType: Replace
platform:
aws:
instanceType: m5.large
subnet:
id: subnet-xxxxxxxxx # Replace with your subnet ID
placement:
spotMarketOptions:
maxPrice: "0.10" # Maximum price per hour in USD (optional)
release:
image: <your-release-image>
EOF
```

### Option 2: Update existing NodePool to use Spot instances

You can also modify an existing NodePool to use Spot instances:

```sh
oc patch nodepool <nodepool-name> -n clusters --type='merge' -p='
{
"spec": {
"platform": {
"aws": {
"placement": {
"spotMarketOptions": {
"maxPrice": "0.10"
}
}
}
}
}
}'
```

### Option 3: Remove Spot configuration from NodePool

To convert a Spot instance NodePool back to On-Demand instances:

```sh
oc patch nodepool <nodepool-name> -n clusters --type='json' -p='
[
{
"op": "remove",
"path": "/spec/platform/aws/placement/spotMarketOptions"
}
]'
```

## Spot instance configuration options

The `spotMarketOptions` field supports the following configuration:

- **maxPrice** (optional): The maximum price per hour that you're willing to pay for a Spot instance, specified in USD. If omitted, the On-Demand price is used as the maximum price.

### Compatibility constraints

Spot instances have the following compatibility requirements:

- **Not compatible** with `spec.platform.aws.placement.capacityReservation` - Spot instances cannot use capacity reservations
- **Requires** `spec.platform.aws.placement.tenancy` to be `default` - Spot instances are not supported with `dedicated` or `host` tenancy

These constraints are enforced through validation rules. Attempting to create a NodePool with incompatible configurations will result in a validation error.

## Understanding Spot instance behavior

### Cost savings

AWS Spot instances can provide significant cost savings, often 50-90% less than On-Demand pricing, depending on the instance type and availability.

### Availability and interruptions

- Spot instances are subject to interruption when AWS needs the capacity back for On-Demand customers
- Instances receive a 2-minute warning before termination
- HyperShift automatically handles instance replacement when Spot instances are terminated through the NodePool's `autoRepair` functionality

### Automatic instance replacement

When a Spot instance is terminated:

1. The Kubernetes node becomes `NotReady`
2. The NodePool controller detects the failed node
3. If `autoRepair: true` is set, a replacement instance is automatically requested
4. Pods are automatically rescheduled to available nodes

## Best practices

### 1. Set appropriate max price

Consider setting a maximum price to control costs, but be aware that setting it too low may result in frequent interruptions:

```yaml
spotMarketOptions:
maxPrice: "0.15" # Set based on your cost tolerance
```

### 2. Use diverse instance types

Consider using multiple NodePools with different instance types to increase availability and reduce the likelihood of simultaneous interruptions across all nodes.

### 3. Design for fault tolerance

Ensure your applications can tolerate node interruptions:

- Use appropriate pod disruption budgets
- Configure adequate replica counts for critical workloads
- Implement proper readiness and liveness probes
- Design stateless applications when possible

### 4. Enable auto-repair

Always enable `autoRepair` in your NodePool management configuration to ensure automatic replacement of interrupted instances:

```yaml
management:
autoRepair: true
upgradeType: Replace
```

### 5. Monitor costs and interruptions

- Regularly monitor your Spot instance usage and costs through the AWS console
- Set up CloudWatch alarms for Spot instance interruptions
- Track NodePool events to understand interruption patterns

## Verification

Verify that your nodes are running as Spot instances:

```sh
# Check the NodePool status
oc get nodepool -n clusters

# Check the nodes in the hosted cluster
oc get nodes -o wide

# Verify instance types and Spot status in AWS console or CLI
aws ec2 describe-instances --region <your-region> --filters "Name=instance-lifecycle,Values=spot" --query 'Reservations[].Instances[].{InstanceId:InstanceId,InstanceType:InstanceType,SpotInstanceRequestId:SpotInstanceRequestId,State:State.Name}'
```

## Troubleshooting

### Spot instance unavailability

If Spot instances are frequently unavailable:

1. Consider increasing your maximum price
2. Try different instance types or sizes
3. Use multiple availability zones
4. Check AWS Spot instance pricing history and availability

### Node replacement issues

If nodes are not being replaced after Spot interruptions:

1. Check the NodePool status: `oc describe nodepool <nodepool-name> -n clusters`
2. Verify that `autoRepair` is enabled in the NodePool management configuration
3. Check the HyperShift operator logs for any errors
4. Ensure the AWS service-linked role for Spot exists in your account

### Validation errors

If you encounter validation errors when creating Spot NodePools:

1. Ensure you're not using dedicated or host tenancy with Spot instances
2. Remove any capacity reservation configuration when using Spot instances
3. Check that your `maxPrice` format is valid (numeric string in USD)

### Monitoring Spot interruptions

Monitor Spot instance interruption events:

```sh
# Check NodePool events
oc describe nodepool <nodepool-name> -n clusters

# Check for node replacement events
oc get events -n clusters --field-selector involvedObject.kind=NodePool
```

For more troubleshooting information, see the [general troubleshooting guide](../troubleshooting-general.md). No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Run docs linters and fix residual Markdown nits (wrap/blank lines/trailing newline).

Please run codespell/markdownlint and address MD013/MD031/MD032/MD047 if reported.


🏁 Script executed:

#!/bin/bash
# Verify formatting and spelling for this page
make verify-codespell || true
npx -y markdownlint-cli2 docs/content/how-to/automated-machine-management/aws-spot-instances.md || true

Length of output: 3266


Fix Markdown lint errors

Running npx markdownlint-cli2 docs/content/how-to/automated-machine-management/aws-spot-instances.md reports the following errors:

  • MD013: lines exceed 80 characters
  • MD007: incorrect list indentation
  • MD031: fenced code blocks not surrounded by blank lines
  • MD047: missing single trailing newline

Please resolve these in the file and re-run the docs linters.

🧰 Tools
🪛 LanguageTool

[grammar] ~11-~11: There might be a mistake here.
Context: ...xisting HyperShift hosted cluster on AWS - Access to the management cluster where t...

(QB_NEW_EN)


[grammar] ~12-~12: There might be a mistake here.
Context: ...where the NodePool resources are created - Ensure that the AWS service-linked role ...

(QB_NEW_EN)


[grammar] ~121-~121: There might be a mistake here.
Context: ...inated: 1. The Kubernetes node becomes NotReady 2. The NodePool controller detects the fail...

(QB_NEW_EN)


[style] ~124-~124: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ... is automatically requested 4. Pods are automatically rescheduled to available nodes ## Best...

(ADVERB_REPETITION_PREMIUM)


[grammar] ~128-~128: There might be a mistake here.
Context: ...ilable nodes ## Best practices ### 1. Set appropriate max price Consider setting...

(QB_NEW_EN)

🪛 markdownlint-cli2 (0.17.2)

14-14: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


18-18: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

🤖 Prompt for AI Agents
In docs/content/how-to/automated-machine-management/aws-spot-instances.md lines
1-221, fix markdownlint issues: wrap or shorten lines to <=80 characters (break
long paragraphs and long command/example lines), correct list indentation to use
consistent two or four-space indenting for nested list items, ensure every
fenced code block is preceded and followed by a blank line, and add exactly one
trailing newline at EOF; after edits re-run npx markdownlint-cli2 to verify no
MD013, MD007, MD031, or MD047 errors remain.

Comment on lines +2533 to +2565
###AWSSpotMarketOptions { #hypershift.openshift.io/v1beta1.AWSSpotMarketOptions }
<p>
(<em>Appears on:</em>
<a href="#hypershift.openshift.io/v1beta1.PlacementOptions">PlacementOptions</a>)
</p>
<p>
<p>AWSSpotMarketOptions defines configuration for AWS Spot instances</p>
</p>
<table>
<thead>
<tr>
<th>Field</th>
<th>Description</th>
</tr>
</thead>
<tbody>
<tr>
<td>
<code>maxPrice</code></br>
<em>
string
</em>
</td>
<td>
<em>(Optional)</em>
<p>maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
If omitted, the On‑Demand price is used as the ceiling.
Example: &ldquo;0.0739&rdquo;
Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless the value is &ldquo;0&rdquo;; scientific notation is not allowed.</p>
</td>
</tr>
</tbody>
</table>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Ensure format constraints match CRD validation and AWS EC2 docs.

Please verify the documented “Format: up to 10 integer digits and up to 6 fractional digits” exactly matches the CEL/regex used in the CRD and aligns with AWS Spot MaxPrice expectations for Launch Templates/Requests.

You can run a quick check across the repo to compare docs vs. schema:


🏁 Script executed:

#!/bin/bash
# Find CRD schema/CEL for maxPrice and compare against docs.
rg -nP "spotMarketOptions|maxPrice|AWSSpotMarketOptions" -C3
rg -nP "pattern|x-kubernetes|cel|XValidation" -g "*nodepools*.crd.yaml" -C3

Length of output: 282509


Format matches CRD schema, but AWS enforces a minimum

  • The docs’ “up to 10 integer digits and up to 6 fractional digits; no leading zeros unless the value is ‘0’; scientific notation is not allowed” is implemented exactly by the CRD’s
      pattern: ^(0|[1-9][0-9]{0,9})(\.[0-9]{1,6})?$
      maxLength: 17
    (matches aws.go lines 69–71) (docs.aws.amazon.com)
  • However, AWS requires any specified Spot maxPrice to be > USD 0.001 (values below 0.001 yield an InvalidParameterValue error) (docs.aws.amazon.com)
  • Add a note in the docs (and/or enforce via CRD XValidation) that maxPrice, if set, must exceed 0.001 to avoid AWS API rejection

Comment on lines +330 to +337
if nodePool.Spec.Platform.AWS.Placement != nil && nodePool.Spec.Platform.AWS.Placement.SpotMarketOptions != nil {
if nodePool.Spec.Platform.AWS.Placement.Tenancy != "" && nodePool.Spec.Platform.AWS.Placement.Tenancy != ec2.TenancyDefault {
return fmt.Errorf("spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')")
}
if nodePool.Spec.Platform.AWS.Placement.CapacityReservation != nil {
return fmt.Errorf("spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')")
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix copy-paste in validation messages; make errors precise.

Tenancy and CapacityReservation failures currently share the same text. Emit targeted messages to aid users.

 if nodePool.Spec.Platform.AWS.Placement != nil && nodePool.Spec.Platform.AWS.Placement.SpotMarketOptions != nil {
     if nodePool.Spec.Platform.AWS.Placement.Tenancy != "" && nodePool.Spec.Platform.AWS.Placement.Tenancy != ec2.TenancyDefault {
-        return fmt.Errorf("spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')")
+        return fmt.Errorf("spotMarketOptions requires placement.tenancy to be 'default' or unset (not 'dedicated' or 'host')")
     }
     if nodePool.Spec.Platform.AWS.Placement.CapacityReservation != nil {
-        return fmt.Errorf("spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')")
+        return fmt.Errorf("spotMarketOptions cannot be used with placement.capacityReservation")
     }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if nodePool.Spec.Platform.AWS.Placement != nil && nodePool.Spec.Platform.AWS.Placement.SpotMarketOptions != nil {
if nodePool.Spec.Platform.AWS.Placement.Tenancy != "" && nodePool.Spec.Platform.AWS.Placement.Tenancy != ec2.TenancyDefault {
return fmt.Errorf("spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')")
}
if nodePool.Spec.Platform.AWS.Placement.CapacityReservation != nil {
return fmt.Errorf("spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')")
}
}
if nodePool.Spec.Platform.AWS.Placement != nil && nodePool.Spec.Platform.AWS.Placement.SpotMarketOptions != nil {
if nodePool.Spec.Platform.AWS.Placement.Tenancy != "" && nodePool.Spec.Platform.AWS.Placement.Tenancy != ec2.TenancyDefault {
return fmt.Errorf("spotMarketOptions requires placement.tenancy to be 'default' or unset (not 'dedicated' or 'host')")
}
if nodePool.Spec.Platform.AWS.Placement.CapacityReservation != nil {
return fmt.Errorf("spotMarketOptions cannot be used with placement.capacityReservation")
}
}
🤖 Prompt for AI Agents
In hypershift-operator/controllers/nodepool/aws.go around lines 330 to 337, the
validation error messages for tenancy and capacityReservation are identical due
to copy-paste; change them to distinct, precise messages: when Tenancy is
invalid return an error that explicitly mentions tenancy must be 'default' or
unset and cannot be 'dedicated' or 'host' for spotMarketOptions, and when
CapacityReservation is set return an error that explicitly states
spotMarketOptions is incompatible with capacityReservation (mention
capacityReservation cannot be used with spotMarketOptions). Ensure the messages
are clear and targeted to each specific check.

Implements complete AWS Spot instance functionality for HyperShift NodePools
with proper validation, extensive testing, and detailed documentation.

API enhancements:
- Add AWSSpotMarketOptions with configurable maxPrice field
- Extend PlacementOptions to include spotMarketOptions
- Implement CEL validation rules preventing incompatible configurations
- Update all generated client code and CRD manifests across feature gates

Controller implementation:
- Integrate Spot instance configuration into AWS nodepool controller

Validation and constraints:
- Prevent usage with capacity reservations and dedicated/host tenancy
- Enforce compatibility requirements through Kubernetes validation
- Add comprehensive unit tests covering edge cases and validation scenarios

Documentation and testing:
- Add detailed usage guide with examples and best practices

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml (1)

498-505: Regenerate after replacing non-ASCII hyphens in Go comments

Description shows non-ASCII hyphens (“instance‑hour”, “On‑Demand”). Replace them in api/hypershift/v1beta1/aws.go comments and re-run codegen so CRDs contain ASCII “-”.

cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml (1)

498-505: Regenerate after replacing non-ASCII hyphens in Go comments

Same non-ASCII hyphens here; fix in source comments and regenerate.

🧹 Nitpick comments (7)
api/hypershift/v1beta1/aws.go (2)

64-66: Use ASCII hyphens in comments to avoid non-ASCII characters in generated CRDs

The comments contain non-ASCII hyphens (e.g., “instance‑hour”, “On‑Demand”). These leak into CRD descriptions and can trip tooling. Replace with ASCII "-".

-// maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
-// If omitted, the On‑Demand price is used as the ceiling.
+// maxPrice defines the maximum price (USD per instance-hour) you are willing to pay for a Spot instance.
+// If omitted, the On-Demand price is used as the ceiling.

Also applies to: 69-71


69-71: Verify MaxPrice validation tags match the vendored API to avoid drift

Pattern/MaxLength here (10 integer + up to 6 fractional digits; MaxLength=17) may differ from the vendored copy historically using a simpler pattern and MaxLength=15. Please confirm both sides match; regenerate CRDs if you change it.

I can provide a follow-up patch once you confirm which spec is authoritative.

hypershift-operator/controllers/nodepool/aws.go (2)

169-174: Clear CapacityReservation fields when enabling Spot to avoid conflicting template output

If placement had CapacityReservation set above, we’ll emit both CapacityReservation and Spot fields. Clear the CR-related fields before setting Spot for consistency.

 if nodePool.Spec.Platform.AWS.Placement != nil && nodePool.Spec.Platform.AWS.Placement.SpotMarketOptions != nil {
+    // Clear mutually exclusive CapacityReservation settings when using Spot.
+    awsMachineTemplateSpec.Template.Spec.CapacityReservationID = nil
+    awsMachineTemplateSpec.Template.Spec.CapacityReservationPreference = ""
     awsMachineTemplateSpec.Template.Spec.SpotMarketOptions = &capiaws.SpotMarketOptions{
         MaxPrice: nodePool.Spec.Platform.AWS.Placement.SpotMarketOptions.MaxPrice,
     }
     awsMachineTemplateSpec.Template.Spec.MarketType = capiaws.MarketTypeSpot
 }

330-337: Use precise, targeted validation messages (avoid conflating tenancy and capacityReservation)

Emit specific errors for each failed check; this improves UX and makes tests less brittle.

 if nodePool.Spec.Platform.AWS.Placement != nil && nodePool.Spec.Platform.AWS.Placement.SpotMarketOptions != nil {
     if nodePool.Spec.Platform.AWS.Placement.Tenancy != "" && nodePool.Spec.Platform.AWS.Placement.Tenancy != ec2.TenancyDefault {
-        return fmt.Errorf("spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')")
+        return fmt.Errorf("spotMarketOptions requires placement.tenancy to be 'default' or unset (not 'dedicated' or 'host')")
     }
     if nodePool.Spec.Platform.AWS.Placement.CapacityReservation != nil {
-        return fmt.Errorf("spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')")
+        return fmt.Errorf("spotMarketOptions cannot be used with placement.capacityReservation")
     }
 }

Also align any test assertions to these exact strings.

docs/content/how-to/automated-machine-management/aws-spot-instances.md (3)

13-21: Fix list indentation and fenced code block nesting (MD007/MD046)

Indent list items by 2 spaces and nest fenced blocks under the bullet.

-    - You can verify if the role already exists using the following command:
-  ```sh
-  aws iam get-role --role-name AWSServiceRoleForEC2Spot
-  ```
-    - If the role does not exist, create it with:
-  ```sh
-  aws iam create-service-linked-role --aws-service-name spot.amazonaws.com
-  ```
+  - You can verify if the role already exists using the following command:
+    ```sh
+    aws iam get-role --role-name AWSServiceRoleForEC2Spot
+    ```
+  - If the role does not exist, create it with:
+    ```sh
+    aws iam create-service-linked-role --aws-service-name spot.amazonaws.com
+    ```

94-94: Align maxPrice documentation with CRD validation (digits/precision examples)

Document the exact accepted format to match API validation; add valid/invalid examples.

-- **maxPrice** (optional): The maximum price per hour that you're willing to pay for a Spot instance, specified in USD. If omitted, the On-Demand price is used as the maximum price.
+- **maxPrice** (optional): The maximum price per hour (USD). If omitted, the On-Demand price is used.
+  Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless exactly "0".
+  Examples: "0", "0.0739", "1234567890.123456". Invalid: "00.10", "1.", ".5", "1.1234567".
-3. Check that your `maxPrice` format is valid (numeric string in USD)
+3. Check that your `maxPrice` format is valid (numeric string in USD, up to 10 integer digits and 6 fractional digits; no leading zeros unless "0")

Also applies to: 206-208


1-221: Run docs linters (wrap/blank lines/trailing newline) to catch residual nits

Address MD013/MD031/MD032/MD047 if reported after your last edits.

I can run markdownlint/codespell locally and push a formatting-only patch if helpful.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 990dbe8 and 12ee78c.

📒 Files selected for processing (19)
  • api/hypershift/v1beta1/aws.go (2 hunks)
  • api/hypershift/v1beta1/zz_generated.deepcopy.go (2 hunks)
  • api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yaml (2 hunks)
  • api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yaml (2 hunks)
  • client/applyconfiguration/hypershift/v1beta1/awsspotmarketoptions.go (1 hunks)
  • client/applyconfiguration/hypershift/v1beta1/placementoptions.go (2 hunks)
  • client/applyconfiguration/utils.go (1 hunks)
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml (2 hunks)
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml (2 hunks)
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml (2 hunks)
  • docs/content/how-to/automated-machine-management/aws-spot-instances.md (1 hunks)
  • docs/content/reference/api.md (2 hunks)
  • hypershift-operator/controllers/nodepool/aws.go (4 hunks)
  • hypershift-operator/controllers/nodepool/aws_test.go (4 hunks)
  • test/e2e/create_cluster_test.go (1 hunks)
  • test/e2e/nodepool_spotmarketoptions_test.go (1 hunks)
  • test/e2e/nodepool_test.go (1 hunks)
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.go (2 hunks)
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
  • test/e2e/create_cluster_test.go
  • test/e2e/nodepool_spotmarketoptions_test.go
  • client/applyconfiguration/hypershift/v1beta1/awsspotmarketoptions.go
  • client/applyconfiguration/hypershift/v1beta1/placementoptions.go
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.go
  • client/applyconfiguration/utils.go
  • test/e2e/nodepool_test.go
  • api/hypershift/v1beta1/zz_generated.deepcopy.go
  • api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yaml
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml
  • api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yaml
  • hypershift-operator/controllers/nodepool/aws_test.go
  • vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go
  • docs/content/reference/api.md
🧰 Additional context used
📓 Path-based instructions (8)
**/!(*.pb).go

📄 CodeRabbit inference engine (.cursor/rules/100-go-mistakes.mdc)

**/!(*.pb).go: Avoid variable shadowing
Do not over-nest control flow (e.g., nested if or for blocks)
Avoid init() functions unless absolutely necessary
Keep functions small and focused
Prefer composition over inheritance (via embedding)
Use the functional options pattern for constructors where flexibility is needed
Avoid defining interfaces until you need them
Do not return interfaces from constructors or public APIs
Define interfaces on the consumer side, not the producer side
Keep interfaces small and focused (generally 1–2 methods)
Avoid embedding pointer types unless necessary
Don’t overuse getters/setters — prefer public fields when it makes sense
Use value receivers when the method doesn't mutate state or require pointer semantics
Do not use util, common, or similarly vague package names
Avoid package name collisions by using clear, unique names
Do not expose unnecessary symbols (keep exported API minimal)
Distinguish between nil and empty slices
Avoid memory leaks from slicing large arrays
Always check the capacity when copying or appending slices
Preallocate slice capacity when size is known ahead of time
Always initialize maps before use
Check existence with the two-value assignment (val, ok := m[key])
Be aware that ranging over a map is in random order
Always check errors — don’t ignore them
Wrap errors with context when rethrowing
Avoid panics except in truly unrecoverable cases
Use errors.Is and errors.As for error comparison in Go 1.20+
Always defer cancel() when using context.WithCancel
Do not leak goroutines — ensure they exit cleanly
Avoid data races — use mutexes or channels appropriately
Never close a channel from the receiving side
Keep imports grouped and ordered: stdlib, external, internal
Avoid magic numbers — use named constants
Prefer explicit over implicit — especially in exported APIs
Only use generics when they simplify code or add real flexibility
Avoid over-engineering with type parameters
Be cautious with constraint complexity — keep...

Files:

  • hypershift-operator/controllers/nodepool/aws.go
  • api/hypershift/v1beta1/aws.go
**/*.go

📄 CodeRabbit inference engine (.cursor/rules/code-formatting.mdc)

Use make lint-fix after writing Go code to automatically fix most linting issues

Follow the rules defined in @100-go-mistakes.mdc for Go code

Files:

  • hypershift-operator/controllers/nodepool/aws.go
  • api/hypershift/v1beta1/aws.go
hypershift-operator/controllers/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Place operator controller implementations under hypershift-operator/controllers/

Files:

  • hypershift-operator/controllers/nodepool/aws.go
{hypershift-operator,control-plane-operator}/controllers/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

{hypershift-operator,control-plane-operator}/controllers/**/*.go: Controller code should follow controller-runtime patterns with proper error handling and requeuing
Use controller-runtime structured logging in controllers

Files:

  • hypershift-operator/controllers/nodepool/aws.go
{hypershift-operator,control-plane-operator}/controllers/**

📄 CodeRabbit inference engine (AGENTS.md)

Place platform-specific implementations within their respective controller subdirectories to keep platform logic isolated

Files:

  • hypershift-operator/controllers/nodepool/aws.go
api/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

After modifying API types in the api/ package, run make api to regenerate APIs and CRDs

Files:

  • api/hypershift/v1beta1/aws.go
api/**

📄 CodeRabbit inference engine (AGENTS.md)

api/**: API definitions and CRDs must reside under the api/ directory
Prefer API version v1beta1; use feature gates for experimental functionality
Generate CRDs via controller-gen using the OpenShift-specific tooling for this repository

Files:

  • api/hypershift/v1beta1/aws.go
**/*.md

📄 CodeRabbit inference engine (.cursor/rules/code-formatting.mdc)

For markdown files, use make verify-codespell to catch spelling errors

Files:

  • docs/content/how-to/automated-machine-management/aws-spot-instances.md
🧠 Learnings (1)
📚 Learning: 2025-08-28T17:26:17.059Z
Learnt from: maxcao13
PR: openshift/hypershift#0
File: :0-0
Timestamp: 2025-08-28T17:26:17.059Z
Learning: The Karpenter provisioner check pattern with potential nil pointer dereference appears in 4 locations in the hypershift codebase: test/e2e/util/util.go, hypershift-operator/controllers/nodepool/token.go, hypershift-operator/controllers/hostedcluster/karpenter.go, and control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go. A 5th location in control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/component.go already implements proper nil checks.

Applied to files:

  • hypershift-operator/controllers/nodepool/aws.go
🪛 LanguageTool
docs/content/how-to/automated-machine-management/aws-spot-instances.md

[grammar] ~11-~11: There might be a mistake here.
Context: ...xisting HyperShift hosted cluster on AWS - Access to the management cluster where t...

(QB_NEW_EN)


[grammar] ~12-~12: There might be a mistake here.
Context: ...where the NodePool resources are created - Ensure that the AWS service-linked role ...

(QB_NEW_EN)


[grammar] ~121-~121: There might be a mistake here.
Context: ...inated: 1. The Kubernetes node becomes NotReady 2. The NodePool controller detects the fail...

(QB_NEW_EN)


[style] ~124-~124: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ... is automatically requested 4. Pods are automatically rescheduled to available nodes ## Best...

(ADVERB_REPETITION_PREMIUM)


[grammar] ~128-~128: There might be a mistake here.
Context: ...ilable nodes ## Best practices ### 1. Set appropriate max price Consider setting...

(QB_NEW_EN)

🪛 markdownlint-cli2 (0.17.2)
docs/content/how-to/automated-machine-management/aws-spot-instances.md

14-14: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


18-18: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
🔇 Additional comments (6)
api/hypershift/v1beta1/aws.go (2)

76-76: LGTM: CEL rule matches the documented constraints

The cross-field validation enforces both mutual exclusion with capacityReservation and tenancy default/unset correctly.


100-104: Field placement and wording read well

spotMarketOptions under PlacementOptions with clear mutual-exclusion doc is appropriate.

hypershift-operator/controllers/nodepool/aws.go (2)

10-11: OK to reference ec2 tenancy constants

Import is minimal and avoids magic strings.


312-314: Guard looks fine if this path is AWS-only

Returning early when AWS is nil is acceptable given this validator is only invoked for AWS. If used more broadly, consider skipping instead of erroring.

cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml (1)

528-533: LGTM: CEL message matches the rule (“default or unset”)

The message now reflects that tenancy may be unset or 'default', aligned with the rule.

cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml (1)

528-533: LGTM: Validation message aligns with CEL rule

Consistent with Default CRD variant.

@gdbranco

gdbranco commented Sep 4, 2025

Copy link
Copy Markdown
Author

/test e2e-aks-4-20

/test okd-scos-e2e-aws-ovn

@gdbranco

gdbranco commented Sep 4, 2025

Copy link
Copy Markdown
Author

/test e2e-aks-4-20

@gdbranco
gdbranco requested review from enxebre and muraee September 5, 2025 13:51
@muraee

muraee commented Sep 5, 2025

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2025
@bryan-cox

Copy link
Copy Markdown
Member

This shouldn't be linked to an OCPSTRAT

// AWSSpotMarketOptions defines configuration for AWS Spot instances
type AWSSpotMarketOptions struct {
// maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
// If omitted, the On‑Demand price is used as the ceiling.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// If omitted, the On‑Demand price is used as the ceiling.

How can I express intent for this with this API?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, I was thinking of omitzero. I guess this is a legit case to use omitempty and not omitzero cc @JoelSpeed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historically, spot markets could exceed the on-demand price, is that still the case?

What is the use case for 0 for the maximum price? Does AWS allow a 0 max price?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"If you specify a maximum price, it must be more than USD $0.001. Specifying a value below USD $0.001 will result in an InvalidParameterValue error message."
https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_SpotMarketOptions.html

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your pattern presently allows 0 as a valid choice, please update to remove that


- Spot instances are subject to interruption when AWS needs the capacity back for On-Demand customers
- Instances receive a 2-minute warning before termination
- HyperShift automatically handles instance replacement when Spot instances are terminated through the NodePool's `autoRepair` functionality

@enxebre enxebre Sep 8, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Instances receive a 2-minute warning before termination

In standalone we run a termination handler which watches this events, mark the machines "to be terminated" and let mhc trigger draining/deletion early as a best effort right before aws terminate them. That's not plumbed here yet and so expectations should be set accordingly for consumers of this feature. Is there a jira ticket to track that as part of this epic

  • HyperShift automatically handles instance replacement when Spot instances are terminated through the NodePool's autoRepair functionality

autoRepair is an opt-in features, so this is only true IF autoRepair is enabled. Let's please clarify that here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@typeid you might want to have a look at this PR from classic vs hcp sre pov

nodePool.Spec.Platform.AWS.Placement = &hyperv1.PlacementOptions{
Tenancy: "default",
SpotMarketOptions: &hyperv1.AWSSpotMarketOptions{
MaxPrice: ptr.To("0.50"),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how likely is this to cause flakes because of spot unavailability?
Is the error message bubbled up to nodepool status via NodePoolAllMachinesReadyConditionType?
If so can we also check that and consider it part of the success criteria for the test.
If it's not bubbled up, is there a jira ticket to track that as part of this epic?

@enxebre

enxebre commented Sep 8, 2025

Copy link
Copy Markdown
Member

thanks! lgtm overall pending addressing jira comments and #6707 (comment)

// AWSSpotMarketOptions defines configuration for AWS Spot instances
type AWSSpotMarketOptions struct {
// maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
// If omitted, the On‑Demand price is used as the ceiling.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historically, spot markets could exceed the on-demand price, is that still the case?

What is the use case for 0 for the maximum price? Does AWS allow a 0 max price?

Comment on lines +100 to +104
// spotMarketOptions specifies options for using AWS Spot instances.
// When specified, instances will be launched as Spot instances with the given configuration.
// Mutually exclusive with capacityReservation, and tenancy must be unset or set to "default".
// +optional
SpotMarketOptions *AWSSpotMarketOptions `json:"spotMarketOptions,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it's copying a very old API which I suspect wouldn't be the way we would recommend implementing this API if we were to go through the API review process today

In particular, it's awkward to use spotMarketOptions: {} as a way to enable a spot instance.

Really you need a discriminated union, which would include the capacity reservation options as well.

If you look at the way MAPI implements this, we have one of the MarketType options as Spot

The ideal here would have been

placement:
  market:
    type: OnDemand | Spot | CapacityBlocks
    capacityBlocks: // Valid only when type is CapacityBlocks
      id: ...
    spot: // Valid only when type is Spot
      maxPrice: ...

I'm not sure why MarketType ended up inside the CapacityReservationOptions here, we really should have looked at how this looked in MAPI and predicted this need 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can either keep it as in the PR or mark CapacityReservation.MarketType as deprecated and expose a new one within PlacementOptions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're happy with a deprecated field, I think it would make more sense to restructure this slightly. We should be able to validate at admission time that the two fields are not different if the existing CapacityReservation.MarketType is present

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what is the direction to follow on this, I agree having the market type directly on placement makes more sense as well

@celebdor celebdor changed the title OCPSTRAT-1677 | feat: allow for aws spot market options on node pools OCPSTRAT-1677: feat: allow for aws spot market options on node pools Sep 9, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 9, 2025
@openshift-ci-robot

openshift-ci-robot commented Sep 9, 2025

Copy link
Copy Markdown

@gdbranco: This pull request references OCPSTRAT-1677 which is a valid jira issue.

Details

In response to this:

feat(aws): add AWS Spot instance support to NodePools

This commit introduces comprehensive support for AWS Spot instances in HyperShift NodePools:

  • Add AWSSpotMarketOptions type with maxPrice configuration
  • Implement validation rules to prevent incompatible placement configurations
  • Update generated client code and API documentation
  • Add comprehensive end-to-end tests for validation scenarios
  • Include detailed documentation for Spot instance usage

Key features:

  • Optional maxPrice field to control maximum Spot instance pricing
  • Automatic compatibility validation (no dedicated tenancy or capacity reservations)
  • Integration with existing NodePool auto-repair functionality for instance replacement
  • Support for cost-effective node provisioning with proper interruption handling
    Fixes #OCPSTRAT-1677

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

Summary by CodeRabbit

  • New Features

  • Add AWS Spot support for NodePools via spotMarketOptions with optional maxPrice (validated numeric string); NodePools launched as Spot will be recognized and treated accordingly.

  • Validation

  • Enforce that spotMarketOptions is incompatible with Capacity Reservations and requires tenancy to be unset or "default"; explicit validation messages guide users.

  • Documentation

  • New how-to guide for running NodePools on AWS Spot, examples, best practices, and verification steps.

  • Tests

  • Expanded unit, integration, and e2e tests covering Spot behavior, tenancy rules, and incompatibility cases.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@enxebre

enxebre commented Sep 9, 2025

Copy link
Copy Markdown
Member

/hold
until there's a path forward for the termination handler implementation and lifecycle

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2025
@openshift-ci

openshift-ci Bot commented Dec 15, 2025

Copy link
Copy Markdown
Contributor

@gdbranco: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-upgrade-hypershift-operator 12ee78c link true /test e2e-aws-upgrade-hypershift-operator
ci/prow/e2e-aks-4-20 12ee78c link true /test e2e-aks-4-20
ci/prow/unit 12ee78c link true /test unit
ci/prow/e2e-kubevirt-aws-ovn-reduced 12ee78c link true /test e2e-kubevirt-aws-ovn-reduced
ci/prow/e2e-aws-4-20 12ee78c link true /test e2e-aws-4-20
ci/prow/e2e-aws 12ee78c link true /test e2e-aws
ci/prow/e2e-aks 12ee78c link true /test e2e-aks
ci/prow/e2e-aws-4-21 12ee78c link true /test e2e-aws-4-21
ci/prow/e2e-aks-4-21 12ee78c link true /test e2e-aks-4-21

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2025
@openshift-merge-robot

Copy link
Copy Markdown
Contributor

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

enxebre referenced this pull request Feb 2, 2026
@enxebre

enxebre commented Feb 3, 2026

Copy link
Copy Markdown
Member

/close
in favour of
#7625
#7567

@enxebre enxebre closed this Feb 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api Indicates the PR includes changes for the API area/cli Indicates the PR includes changes for CLI area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/documentation Indicates the PR includes changes for documentation area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release area/testing Indicates the PR includes changes for e2e testing do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants